Visit binary operators in ref safety analysis by jjonescz · Pull Request #73081 · dotnet/roslyn

jjonescz · 2024-04-18T11:21:19Z

Alternative to #72935.
Fixes #72873.

jaredpar · 2024-04-18T18:03:40Z

src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs

+        }
+
+#if DEBUG
+        private void VisitCore(BoundNode? node)


Given this method is only used for assertion purposes did you consider giving it an assert style named and making it conditional on DEBUG? For example:

[Conditional("DEBUG")] private void AssertVisitCore(BoundNode? node) ``` #Resolved

Making it just Conditional would fail to compile as it uses _visited field which is only available #if DEBUG.

Can use an #if DEBUG within the method too. Personally I find the code easier to process when minimize the number of directives, particularly for call sites. Will defer to others here.

In this case, there are only a couple of call sites, so I think I prefer the explicit-ness of #if DEBUG at the declaration and at the call sites so it's clear at the callers that there is no impact on RELEASE builds.

My first choice in a situation like this will be to rely on conditional compilation vs. the attribute. It completely eliminates the method (including in produced metadata) for release build. Whereas the attribute doesn't have this effect. In my opinion the attribute is useful when we deal with a public API, which is not the case here.

AlekseyTs · 2024-04-19T14:20:26Z

src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs

+        }
+
+#if DEBUG
+        private void VisitCore(BoundNode? node)


VisitCore

I think we should use a name that clearly indicates the purpose of this method. "VisitCore" doesn't tell to me that the purpose is to perform some debug time validation. Quite the opposite, it tells me the method performs some critical task and it is surprising to see that it not called in release build. #Closed

AlekseyTs · 2024-04-19T14:37:16Z

src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs

+            {
+                this.Visit(op.Right);
+#if DEBUG
+                this.VisitCore(op);


this.VisitCore(op);

It looks like, If we were not overriding BoundTreeWalker.VisitBinaryOperator, we would be calling VisitCore for each parent, before visiting any child. It also looks like this is not what this method is doing. It looks like it visits children and then calls VisitCore for the parent. I think that manual stack management should preserve the regular order of operations. #Closed

AlekseyTs · 2024-04-19T14:45:18Z

src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs

        }

+        // Unlike the base implementation, we want to visit all the nodes, not just the operands.
+        public override BoundNode? VisitBinaryOperator(BoundBinaryOperator node)


VisitBinaryOperator

I do not see any good reason to use drastically different algorithm in this method by comparison to BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator.VisitBinaryOperator implementation. This could be explained by desire to call VisitCore on the way "out" rather than on the way "in", but I think we actually want to do that the other way. #Closed

AlekseyTs · 2024-04-19T14:58:42Z

        if (_visited is { } && _visited.Count <= MaxTrackVisited)

At the very least, I think we should call VisitCore here. But I also think that what we should really do is to override VisitCall and have it call VisitCore at the right time instead in order to preserve the regular order of operations. For example, it looks like if we simply call VisitCore here, we would be calling it twice for the top level BoundCall node. #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs:708 in e7f5146. [](commit_id = e7f5146, deletion_comment = False)

AlekseyTs · 2024-04-19T14:59:23Z

Done with review pass (commit 2)

AlekseyTs · 2024-04-19T15:21:03Z

        rightOperands.Push(binary.Right);

As a better alternative to overriding this method in derived type and duplicating the stack management process, I think we can add a protected virtual method called "BeforeVisitingSkippedBoundBinaryOperatorChildren" or something like that and do BeforeVisitingSkippedBoundBinaryOperatorChildren(binary); here and in the loop below. RefSafetyAnalysis would override it and call VisitCore from that method. #Closed

Refers to: src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs:117 in e7f5146. [](commit_id = e7f5146, deletion_comment = False)

AlekseyTs · 2024-04-19T15:22:13Z

            rightOperands.Push(binary.Right);

Here is the second place to call BeforeVisitingSkippedBoundBinaryOperatorChildren(binary); #Closed

Refers to: src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs:124 in e7f5146. [](commit_id = e7f5146, deletion_comment = False)

AlekseyTs · 2024-04-19T15:25:56Z

                calls.Push(node);

Similar to VisitBinaryOperator, here we could call BeforeVisitingSkippedBoundCallChildren(node);, override it RefSafetyAnalysis and call VisitCore there. #Closed

Refers to: src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs:150 in e7f5146. [](commit_id = e7f5146, deletion_comment = False)

AlekseyTs

LGTM (commit 3)

These tests verify that issue dotnet#63852 is fixed. It was very likely fixed as part of dotnet#73081 closes dotnet#63852

* Tests to verify ref safety bug fix These tests verify that issue #63852 is fixed. It was very likely fixed as part of #73081 closes #63852 * formatting issue * Apply suggestions from code review Co-authored-by: Jan Jones <jan.jones.cz@gmail.com> --------- Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>

Add tests

21349e2

ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 18, 2024

Visit binary operators in ref safety analysis

e7f5146

jjonescz force-pushed the 72873-RefSafetyAssert-2 branch from e943f8f to e7f5146 Compare April 18, 2024 11:41

jjonescz marked this pull request as ready for review April 18, 2024 12:54

jjonescz requested a review from a team as a code owner April 18, 2024 12:54

jjonescz requested review from AlekseyTs and cston April 18, 2024 12:54

cston approved these changes Apr 18, 2024

View reviewed changes

jaredpar reviewed Apr 18, 2024

View reviewed changes

AlekseyTs reviewed Apr 19, 2024

View reviewed changes

Simplify visit tracking

b740b2f

cston approved these changes Apr 22, 2024

View reviewed changes

AlekseyTs approved these changes Apr 22, 2024

View reviewed changes

jjonescz merged commit a9b690f into dotnet:main Apr 23, 2024

jjonescz deleted the 72873-RefSafetyAssert-2 branch April 23, 2024 08:38

dotnet-policy-service bot added this to the Next milestone Apr 23, 2024

dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024

jaredpar added a commit to jaredpar/roslyn that referenced this pull request Jul 10, 2024

Tests to verify ref safety bug fix

de581ed

These tests verify that issue dotnet#63852 is fixed. It was very likely fixed as part of dotnet#73081 closes dotnet#63852

jaredpar mentioned this pull request Jul 10, 2024

Tests to verify ref safety bug fix #74341

Merged

jaredpar added a commit to jaredpar/roslyn that referenced this pull request Jul 15, 2024

Tests to verify ref safety bug fix

77b5387

These tests verify that issue dotnet#63852 is fixed. It was very likely fixed as part of dotnet#73081 closes dotnet#63852

Conversation

jjonescz commented Apr 18, 2024

Uh oh!

jaredpar Apr 18, 2024 • edited by jjonescz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

jaredpar Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

cston Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Apr 19, 2024

Uh oh!

AlekseyTs commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jaredpar Apr 18, 2024 •

edited by jjonescz

Loading

AlekseyTs Apr 19, 2024 •

edited

Loading

AlekseyTs Apr 19, 2024 •

edited

Loading

AlekseyTs Apr 19, 2024 •

edited

Loading

AlekseyTs commented Apr 19, 2024 •

edited

Loading

AlekseyTs commented Apr 19, 2024 •

edited

Loading

AlekseyTs commented Apr 19, 2024 •

edited

Loading

AlekseyTs commented Apr 19, 2024 •

edited

Loading